-
-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/transfer routes #595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Feat/transfer routes #595
Conversation
* feat(api): add api routes for /users/me * fix(tests): api me PUT * feat(api): add delete me endpoint
| export const createTransfer = async ( | ||
| boxId: string, | ||
| expiresAt?: Date | ||
| ): Promise<Claim> => { | ||
| const token = generateTransferCode(); | ||
| const expirationDate = expiresAt || getDefaultExpirationDate(); | ||
|
|
||
| const [newClaim] = await drizzleClient | ||
| .insert(claim) | ||
| .values({ | ||
| boxId, | ||
| token, | ||
| expiresAt: expirationDate, | ||
| }) | ||
| .returning(); | ||
|
|
||
| if (!newClaim) { | ||
| throw new Error("Failed to create transfer claim"); | ||
| } | ||
|
|
||
| return newClaim; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be redundant, as there is createBoxTransfer in transfer-service.server.ts.
Same for the other methods in this file. Why do both versions need to exist? 🤔
app/routes/api.claim.ts
Outdated
| if (!contentType || !contentType.includes("application/json")) { | ||
| return Response.json( | ||
| { | ||
| code: "NotAuthorized", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotAuthorized 🤨 Isn't this Unsupported Media Type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is 😅 should be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually i just remembered, i found this example in the opensensemap docs:
HTTP/1.1 415 Unsupported Media Type
{"code":"NotAuthorized","message":"Unsupported content-type. Try application/json"}
which is why i first wrote NotAuthorized, but now that i think about it again we should just fix the code here right? I dont think any would check against the Code being NotAuthorized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see in restify-errors implementation (which is what the old api is using), I think this might even be wrong in the docs 😅
So yes, definitely it should be UnsupportedMediaType.
In fact, when you try it the osem api actually returns that:
![]()
No description provided.